PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.4±0.07ms     4.8 MB/sec    1.06      8.9±0.06ms     4.6 MB/sec
formatter/numpy/ctypeslib.py               1.00  1649.2±25.56µs    10.1 MB/sec    1.05  1728.1±41.31µs     9.6 MB/sec
formatter/numpy/globals.py                 1.00    185.3±2.42µs    15.9 MB/sec    1.02    188.3±1.62µs    15.7 MB/sec
formatter/pydantic/types.py                1.00      3.5±0.11ms     7.3 MB/sec    1.05      3.7±0.07ms     6.9 MB/sec
linter/all-rules/large/dataset.py          1.00     11.0±0.12ms     3.7 MB/sec    1.00     11.0±0.06ms     3.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.9±0.03ms     5.8 MB/sec    1.00      2.9±0.04ms     5.8 MB/sec
linter/all-rules/numpy/globals.py          1.01    402.7±6.75µs     7.3 MB/sec    1.00    399.1±1.13µs     7.4 MB/sec
linter/all-rules/pydantic/types.py         1.02      5.7±0.08ms     4.5 MB/sec    1.00      5.6±0.05ms     4.5 MB/sec
linter/default-rules/large/dataset.py      1.00      5.6±0.03ms     7.3 MB/sec    1.02      5.7±0.03ms     7.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1231.6±19.27µs    13.5 MB/sec    1.01  1242.6±10.99µs    13.4 MB/sec
linter/default-rules/numpy/globals.py      1.01    146.5±4.14µs    20.1 MB/sec    1.00    144.5±2.54µs    20.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.03ms    10.2 MB/sec    1.02      2.6±0.02ms    10.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     10.1±0.11ms     4.0 MB/sec    1.00     10.1±0.12ms     4.0 MB/sec
formatter/numpy/ctypeslib.py               1.00  1937.5±18.08µs     8.6 MB/sec    1.01  1960.5±43.33µs     8.5 MB/sec
formatter/numpy/globals.py                 1.00    218.5±5.41µs    13.5 MB/sec    1.00    218.0±6.51µs    13.5 MB/sec
formatter/pydantic/types.py                1.00      4.2±0.06ms     6.0 MB/sec    1.01      4.2±0.06ms     6.0 MB/sec
linter/all-rules/large/dataset.py          1.00     12.8±0.16ms     3.2 MB/sec    1.01     12.9±0.15ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.04ms     4.7 MB/sec    1.01      3.5±0.03ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00   443.1±14.05µs     6.7 MB/sec    1.01    447.6±9.91µs     6.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.6±0.07ms     3.8 MB/sec    1.01      6.7±0.11ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.00      6.9±0.06ms     5.9 MB/sec    1.01      7.0±0.08ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1480.3±23.45µs    11.2 MB/sec    1.01  1501.1±23.79µs    11.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    176.3±4.32µs    16.7 MB/sec    1.01    178.4±3.74µs    16.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.05ms     8.1 MB/sec    1.00      3.1±0.04ms     8.1 MB/sec

@evanrittenhouse I suspect there are probably too many outstanding Applicability::unspecified fixes for us to stop fixing those automatically yet — although I have not checked in a bit.

I think we may want Applicability::suggested fixes to require a separate opt-in?

Yeah, so by default we'll apply Applicability::Automatic (suggested will in fact require a separate opt-in).. After getting this done (the code part), I'm going to go through and categorize the rest of the fixes. Once the fixes are done, I'll rebase this and then mark it as complete.

I've begun categorizing some of the rules (#5129, #5130, #5127). I plan on doing some more later tonight or tomorrow. This PR depends on all Fix::unspecified instances being refactored

What's the consensus on opting into suggested fixes? I was thinking we could either:

1. Have it be an option in the relevant .toml file
2. Have it be a CLI option (e.g. --fix would only implement Applicability::Automatic fixes, while --fix --include-suggested would implement Applicability::Automatic|Suggested.

Or both!

cc @zanieb @charliermarsh

I don't think we have a consensus yet and is something we should discuss:

Adding a new CLI option seems reasonable to me. I would suggest to prefix the option with --fix, e.g. --fix-suggested, to make its relation clear. We may want to be more explicit about their potential unsafe nature by naming the option --fix-unsafe. However, this has the downside that the wording in the diagnostic and the CLI option do not align. Clippy uses a YOLO environment (not the exact name) to enable all fixes. An alternative is that --fix optionally accepts automatic|yolo (no idea what to call the value xD)

I'm unsure how to support this in the configuration best. I would expect that adding a rule to fixable promotes it to automatic (only when applying, not for rendering diagnostics). Adding a rule to unfixable demotes it to manual. The challenge with this approach is that promoting a single suggested fix requires adding all automatic fixes to fixable, which is rather annoying. We could deprecate the existing fixable and unfixable in favor of adding new options fix.automatic (fixable), fix.suggested, and fix.manual (unfixable).

We should also think about what changes are necessary in the VS code extension and I'm probably missing a few details here.

However, this has the downside that the wording in the diagnostic and the CLI option do not align

Why did we move away from safe/unsafe, etc. to the current model? Could/should we switch back?

An alternative is that --fix optionally accepts automatic|yolo (no idea what to call the value xD)

I like this! We could default to automatic, then allow for suggested and manual (or yolo :P) options to be passed which would adjust the "base" applicability that gets fixed.

would expect that adding a rule to fixable promotes it to automatic (only when applying, not for rendering diagnostics). Adding a rule to unfixable demotes it to manual. The challenge with this approach is that promoting a single suggested fix requires adding all automatic fixes to fixable, which is rather annoying.

I like this approach, though I think we could smooth the implementation by providing an applicability_override field on Fix that gets populated only if the fix is in fixable or unfixable. Then, regardless of applicability level, we could check if the fix's safety level should be overridden. That would allow us to pass a "suggested" diagnostic to fixable and, when we generate a fix for it, check the override and include it in the default fixable set regardless. Does that sound fair?

cc @MichaReiser wondering if the above design makes sense - I plan on wrapping up the remaining linters tonight, then I'll be able to come back to this and implement the rest of the selection logic.

@evanrittenhouse - Micha's gonna be out tomorrow so feel free to ping again on Wednesday if he doesn't get back to you.

@evanrittenhouse you can see the discussion about the names in #4183

I think since the display is "Fix" and "Suggested fix" (as implemented in #4303 but we could change this) users would understand --fix-suggested. --fix-unsafe would not make it clear to me that the "Manual fixes" would not be applied. I also don't mind the idea of --fix [applicability] but I'm not sure how discoverable it is — seems like the way to go if we want to add more applicability levels but that doesn't seem particularly likely.

@zanieb Interesting. So if we were to go with --fix only doing automatic and --fix-suggested doing suggested + automatic, how would the user see manual fixes? Just via --diff?

Also, I'm curious how we want to handle the configuration file for this. In my opinion, adding a rule (regardless of applicability level) to fixable should make it get fixed. Adding a rule (again, regardless of applicability) to unfixable should leave it alone. To avoid changing the Applicability enum, I think that it makes sense to add an applicability_ovverride field to Fix which:

1. Gets parsed at runtime from the appropriate .TOML file
2. Supersedes the Applicability level assigned to the fix
3. Joins/excludes it from the fixable set

What do you think?

I like this approach, though I think we could smooth the implementation by providing an applicability_override field on Fix that gets populated only if the fix is in fixable or unfixable. Then, regardless of applicability level, we could check if the fix's safety level should be overridden. That would allow us to pass a "suggested" diagnostic to fixable and, when we generate a fix for it, check the override and include it in the default fixable set regardless. Does that sound fair?

I'm a bit hesitant from adding new fields to Fix because:

• It increases the memory consumption of every fix.
• Fixes are currently constructed manually by linters. Passing and initializing a field requires changing every single Fix call, increasing the complexity for authoring lint rules.

I'm unsure if I understand your motivation for adding the override field. Is it to avoid generating unnecessary fixes?

how would the user see manual fixes? Just via --diff?

That's correct. Manual fixes should not be applied automatically. I wonder if it even makes sense allowing users to promote manual fixes because it is intentional that manual fixes can be incomplete, always or sometimes resulting in invalid syntax.

To avoid changing the Applicability enum, I think that it makes sense to add an applicability_ovverride field to Fix which:

Would this be initialized depending on whether the rule is in the fixable or unfixable? What are your thoughts on renaming the configuration options to use a single terminology?

Gets parsed at runtime from the appropriate .TOML file

What do you mean by appropriate toml file. Reading it from the closest pyproject toml or is this another configuration file?

I'm unsure if I understand your motivation for adding the override field. Is it to avoid generating unnecessary fixes?

Yes, an attempt to keep the current configuration option setup with fixable/unfixable. If the user passes --fix (e.g., selecting Automatic fixes), but a Suggested rule is in fixable, we need a way to show that the suggested rule should be fixed. As far as the creation goes, I don't actually think it's that bad. The override field would be private and defaulted to whatever Applicability the Fix's construtor specifies. My proposal about the override field was really only in case we wanted to keep the existing setup. For example, how do we handle the case where the user selects fix.automatic but additionally wants to fix only one Applicability::Suggested rule? Do we even want to?

I believe above you also mentioned:

We could deprecate the existing fixable and unfixable in favor of adding new options fix.automatic (fixable), fix.suggested, and fix.manual (unfixable).

Which would also definitely work (and I think is better), but I'm not sure how we'd handle the above case.

That's correct. Manual fixes should not be applied automatically. I wonder if it even makes sense allowing users to promote manual fixes because it is intentional that manual fixes can be incomplete, always or sometimes resulting in invalid syntax.

That's fine with me. I think the important thing is that we make it clear that Manual isn't a typical Fix level, and that we can only show them, never implement them. That's easily done though. :)

What do you mean by appropriate toml file. Reading it from the closest pyproject toml or is this another configuration file?

Yeah, closest pyproject.toml (or ruff.toml, if applicable I guess).

@MichaReiser let me know what you think!

Yes, an attempt to keep the current configuration option setup with fixable/unfixable. If the user passes --fix (e.g., selecting Automatic fixes), but a Suggested rule is in fixable, we need a way to show that the suggested rule should be fixed. As far as the creation goes, I don't actually think it's that bad. The override field would be private and defaulted to whatever Applicability the Fix's construtor specifies.

Which code would be responsible for changing/setting the override field?

My proposal about the override field was really only in case we wanted to keep the existing setup. For example, how do we handle the case where the user selects fix.automatic but additionally wants to fix only one Applicability::Suggested rule? Do we even want to?

Can you elaborate how this would work with the override field?

What I had in mind is that ruff generates all fixes and then filter out the fixes where the Applicability doesn't match. This has the downside that we'll generate more fixes than what's strictly necessary but I don't think I'm too concerned about that. An alternative would be to change Checker.patch to accept an Applicability but we don't always know the applicability ahead of time and it would be easy to change the applicability when generating the fix but not when checking whether a patch needs to be created.

@MichaReiser This is basically how I'm imagining the entire feature.

CLI

1. --fix - changed to fix only Automatic fixes and rules in the fixable set
2. --fix-unsafe - new option, will fix Automatic and Suggested fixes. While it differs from the implementation's naming conventions, as a user "safe" and "unsafe" make more sense, IMO. From the fixer's perspective, an "automatic" or "suggested" fix makes sense. I think it's clearer for users to consider fixes as "this is safe to implement" or "this is unsafe". Manual changes will never be fixed
3. --diff - will output only Automatic fixes and rules in the fixable set
4. --fix-only - will only fix Automatic fixes and rules in the fixable set

Rules with a Manual applicability will never be fixed, nor will rules in the unfixable set.

Configuration File

fixable/unfixable remain as normal, preserving backwards compatibility.

Implementation

Consider the example where we're enforcing default rules (e.g., E, F) and F841 (unused variable).

The Fix gets generated here - assume that we're going with the override field as well. The Fix::suggested() constructor would assign an override: None. The user then runs --fix. Ruff goes out and fixes all Automatic fixes, leaving F841 untouched.

Now assume that the user wants to fix all Automatic fixes, with F841 added. They F841 into the fixable set in their pyproject.toml file. The fixable/unfixable tables would essentially operate as ways to say "I always want this fixed" or "I never want this fixed".

We need to somehow tell F841 that its Applicability has to be overridden. This can be done in a few ways (and thanks for pushing back, I think the override field is worse):

1. An override field. We could add a set_override() method to Fix which allows you to pass an Applicability, setting override: Some(new_applicability). The downside here is going through and adding set_override() calls everywhere, and it adds memory, like you said.
2. preferred: Checker handles all fixing logic itself, now based on the CLI command and the contents of fixable/unfixable. The upside here is no change is required on a per-rule basis - checker.patch(Rule::UnusedVariable) would remain true and generate the fix.

If we go with the second option, Checker would see that the "base applicability" (determined by seeing that the user ran --fix and not --fix-unsafe) is Automatic, and that fixable contains F841. Regardless of applicability, patch() would therefore return true, and we'd generate the fix. All fix generation would be gated through Checker.patch().

What do you think?

Thanks for the in-depth elaboration. This helped.
I think it would be good to create a discussion for the feature. What we're discussing is unrelated to the code changes and
the lack of topics makes the discussion less efficient.

Inputs/Questions from my side:

• --fix-unsafe: I think its good to have the option. We can align on the final naming later.
• --fix-only: Should --fix-unsafe enable fixing the suggested fixes too?
• --diff: Should --fix-unsafe enable the suggested fixes? I think it should be aligned with --fix-only, considering that --diff implies --fix-only.
• --diff: We could show diffs for manual fixes. I'm not recommending that we should do this. We may decide to only show the fixes as part of the MessageEmitters in the future.
• Configuration: How would I promote a single suggested rule?

@MichaReiser Done! I'll be out most of today but will be able to get to your feedback by tonight or tomorrow morning probably.

Hey @evanrittenhouse

I just want to let you know that I'm working on our CLI design right now and I expect it to overlap a bit with our plans for this feature. For that reason, I expect it to be a bit before we're ready to merge this breaking change. I'll be back with some guidance soon hopefully.

Let me know if you have any questions or concerns!

@zanieb no worries, thanks for the heads up! I'll just work on implementing the changes mentioned and leave the CLI option stuff for later. Should be able to get the underlying implementation down at least.

I might go back and see what I can do about the Markdown parser...

CodSpeed Performance Report

Merging #5119 will not alter performance

_{Comparing evanrittenhouse:4185_cli_respect_applicability (05462ce) with main (f32b0ee)}

Summary

✅ 25 untouched benchmarks

Finally getting around to rebasing this, hoping to have it done either early this week.

Replaced by #7769 which used the commits from here. Thanks so much Evan for driving this forward!